fix(aqua): avoid double raw windows extensions by risu729 · Pull Request #10227 · jdx/mise

risu729 · 2026-06-04T21:47:08Z

Summary

keep raw Windows aqua assets/URLs with an existing file extension unchanged before applying windows_ext
continue completing extensionless raw Windows assets/URLs as before
add regression coverage for raw .bat asset and direct URL rendering

Review validation

Validated the remaining CodeRabbit comment from #10167 with a focused reproduction. Before the fix, this allowed registry shape rendered a double extension on Windows:

asset: tool-{{.Version}}-{{.OS}}-{{.Arch}}.bat
format: raw
windows_ext: .bat

For 1.0.0 on windows/amd64, pkg.asset(...) produced tool-1.0.0-windows-amd64.bat.bat instead of tool-1.0.0-windows-amd64.bat. The same issue is observable for direct URLs such as https://example.com/tool/{{.Version}}/tool.bat, which would make mise request tool.bat.bat.

I also checked the current aqua registry: sass/dart-sass uses windows_ext: .bat, but it is archive-based rather than this format: raw shape. So this is not a current dart-sass regression; it is a valid edge case for raw assets/URLs that the registry schema permits.

Upstream aqua comparison

Current aquaproj/aqua at 1e4b1cf0 does not avoid this edge case. Its completeWindowsExtToAsset returns completeWindowsExt(asset) unconditionally for format: raw after only skipping assets ending in .exe or .jar.

I verified that with a temporary local reproduction against aqua's RenderAsset path: the same format: raw / windows_ext: .bat asset rendered as tool-1.0.0-windows-amd64.bat.bat.

Tests

cargo test -p aqua-registry raw_preserves_custom_windows_ext -- --nocapture
cargo test -p aqua-registry
cargo fmt -p aqua-registry --check

Current aqua-registry packages

None. I scanned current aquaproj/aqua-registry at 830124e62b for packages matching the exact risky shape: format: raw, Windows extension completion enabled, a custom windows_ext, and an asset/URL that already carries that extension. No packages match.

The only current windows_ext: .bat package found is sass/dart-sass, and it uses archive formats (zip/tar.gz) rather than format: raw.

Summary by CodeRabbit

Bug Fixes
- Enhanced Windows extension handling for raw-format assets to more conservatively preserve existing file extensions and prevent unintended modifications.
Tests
- Added test coverage for raw assets with custom extensions to verify they remain unmodified during processing.

coderabbitai · 2026-06-04T21:47:15Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: abbc52f2-228f-4962-90cc-54fe47319c09

📥 Commits

Reviewing files that changed from the base of the PR and between bb100fb and 882fa7a.

📒 Files selected for processing (1)

crates/aqua-registry/src/types.rs

📝 Walkthrough

Walkthrough

This PR refines AquaPackage::complete_windows_ext_to_asset to handle raw-format assets more conservatively. When format is raw, the method now checks whether a target string already has a filename extension using os_file_ext_is_empty before appending a Windows extension. If an extension exists, the input is returned unchanged. Two tests validate this behavior for both raw URLs and raw asset strings.

Changes

Raw Format Windows Extension Preservation

Layer / File(s)	Summary
Windows extension logic for raw format `crates/aqua-registry/src/types.rs`	`complete_windows_ext_to_asset` adds a `raw`-specific branch that checks for existing filename extensions and only appends the Windows extension if the string lacks an extension; existing tests for raw URLs and raw assets ending with custom extensions (`.bat`) verify the string is not further modified.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

jdx/mise#10167: Establishes the os_file_ext_is_empty helper and Windows extension semantics that this PR leverages for conservative raw-format handling.

Poem

🐰 Raw formats stay pristine, no extra fluff today,
Extensions already there? We'll just look away,
Conservative and calm, we check before we add,
These tests prove restraint—and that's not bad! 🌱

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(aqua): avoid double raw windows extensions' directly and clearly summarizes the main change—preventing double application of Windows extensions to raw format assets.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

risu729 · 2026-06-04T21:47:45Z

@coderabbitai review

This comment was generated by an AI coding assistant.

coderabbitai · 2026-06-04T21:48:50Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR details and diff
gh pr view 10227 --json title,body,files,additions,deletions
echo "---DIFF---"
gh pr diff 10227

Length of output: 3631

🏁 Script executed:

#!/bin/bash
# Read context around the fix and helper methods
rg -n "os_file_ext_is_empty|complete_windows_ext|windows_ext" crates/aqua-registry/src/types.rs -A 8 -B 2
echo "=== Full context around line 617 ==="
sed -n '600,650p' crates/aqua-registry/src/types.rs
echo "=== asset() and url() method signatures ==="
rg -n "fn asset|fn url|fn complete_windows_ext|fn os_file_ext_is_empty" crates/aqua-registry/src/types.rs

Length of output: 16470

@risu729 The fix looks correct and well-scoped. Here's the full review:

Review of `fix(aqua): avoid double raw windows extensions`

Logic

The change in complete_windows_ext_to_asset is exactly right. Before the fix, the format == "raw" branch unconditionally called complete_windows_ext(s), which appended the Windows extension even when the template-expanded string already carried it. The guard added here:

if self.format == "raw" {
    if self.os_file_ext_is_empty(s, v) {
        return Ok(self.complete_windows_ext(s));
    }
    return Ok(s.to_string());
}

mirrors the identical pattern already used for complete_windows_ext_to_file_src and for the general (empty format) case lower in the same function. The fix is minimal and consistent with the existing decision tree.

Correctness of `os_file_ext_is_empty`

os_file_ext_is_empty operates on the already-template-expanded string (the template has been rendered by the time complete_windows_ext_to_asset is called), so checking the extension of the final filename is reliable. The rsplit('/') correctly strips the URL path prefix before extension detection.

Edge-case interaction with early guards

The .exe and .jar guards at the top of complete_windows_ext_to_asset still fire before reaching the raw branch, so a raw asset ending in .exe or .jar continues to be handled as before.

Test coverage

Both new tests are well-formed:

test_url_raw_preserves_custom_windows_ext — URL path (tool.bat) is not doubled.
test_asset_raw_preserves_custom_windows_ext — template-produced asset name (tool-...-amd64.bat) is not doubled.

The complementary path (extensionless raw asset → extension appended) is already covered by the pre-existing test_asset_uses_custom_windows_ext, so there's no gap.

No concerns

The fix is correct, the tests are sufficient, and the implementation is consistent with the rest of the codebase. ✅

This comment was generated by an AI coding assistant.

🧠 Learnings used

Learnt from: CR
Repo: jdx/mise PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-31T15:12:12.203Z
Learning: When posting comments on GitHub PRs or discussions, include a note that the comment was AI-generated (e.g., '*This comment was generated by an AI coding assistant.*').

Learnt from: CR
Repo: jdx/mise PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-31T15:12:18.847Z
Learning: When posting comments on GitHub PRs or discussions, always include a note that the comment was AI-generated

Learnt from: CR
Repo: jdx/mise PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-31T15:12:28.703Z
Learning: When posting comments on GitHub PRs or discussions, always include a note that the comment was AI-generated

Learnt from: CR
Repo: jdx/mise PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-31T15:12:28.703Z
Learning: Applies to **/*_windows.rs : Windows-specific tests are in e2e-win/ directory and Windows-specific implementations are in files ending with _windows.rs

Learnt from: CR
Repo: jdx/mise PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-31T15:12:18.847Z
Learning: Applies to src/**/*_windows.rs : Use platform-specific file naming with _windows.rs suffix for Windows-specific implementations

Learnt from: CR
Repo: jdx/mise PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-03T17:35:47.414Z
Learning: Prefer `aqua:` backend for registry entries; use `github:` or `gitlab:` when the tool isn't in aqua but ships releases. Avoid tier 3 backends (npm, pipx, gem, cargo, go, dotnet) for registry PRs unless explicitly approved.

greptile-apps · 2026-06-04T21:49:31Z

Greptile Summary

This PR fixes a double-extension bug in the aqua backend where a format: raw asset or URL that already carries a file extension (e.g., .bat) would have the windows_ext value appended a second time on Windows, producing names like tool.bat.bat. The fix aligns the raw format branch with the existing extensionless-detection logic already used for the empty-format path.

complete_windows_ext_to_asset now calls os_file_ext_is_empty before applying complete_windows_ext in the raw format branch, mirroring the identical guard used in the empty-format path below it.
Two new unit tests cover the asset and URL paths for a raw .bat package with windows_ext: .bat, confirming the single-extension outcome.

Confidence Score: 5/5

The change is a narrow, well-tested guard that prevents a double-extension on Windows for raw-format aqua packages; no existing packages in the registry are affected.

The fix is a one-line conditional change that makes the raw format branch consistent with the already-proven empty-format branch; the helper it calls (os_file_ext_is_empty) is well-tested and already used identically elsewhere in the same function. Two targeted regression tests cover both the asset and URL callers.

No files require special attention.

Important Files Changed

Filename	Overview
crates/aqua-registry/src/types.rs	Fixes double-extension bug for raw-format assets/URLs on Windows by skipping `complete_windows_ext` when a file extension already exists; adds two regression tests covering asset and URL paths.

_{Reviews (2): Last reviewed commit: "docs(aqua): clarify raw windows extensio..." | Re-trigger Greptile}

risu729 · 2026-06-05T05:05:16Z

@coderabbitai review

This comment was generated by an AI coding assistant.

coderabbitai · 2026-06-05T05:05:32Z

@risu729 Triggering a fresh review of the PR now.

This comment was generated by an AI coding assistant.

🧠 Learnings used

Learnt from: CR
Repo: jdx/mise PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-31T15:12:12.203Z
Learning: When posting comments on GitHub PRs or discussions, include a note that the comment was AI-generated (e.g., '*This comment was generated by an AI coding assistant.*').

Learnt from: CR
Repo: jdx/mise PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-31T15:12:18.847Z
Learning: When posting comments on GitHub PRs or discussions, always include a note that the comment was AI-generated

Learnt from: CR
Repo: jdx/mise PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-31T15:12:28.703Z
Learning: When posting comments on GitHub PRs or discussions, always include a note that the comment was AI-generated

Learnt from: CR
Repo: jdx/mise PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-31T15:12:28.703Z
Learning: Applies to **/*_windows.rs : Windows-specific tests are in e2e-win/ directory and Windows-specific implementations are in files ending with _windows.rs

Learnt from: CR
Repo: jdx/mise PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-03T17:35:47.414Z
Learning: Prefer `aqua:` backend for registry entries; use `github:` or `gitlab:` when the tool isn't in aqua but ships releases. Avoid tier 3 backends (npm, pipx, gem, cargo, go, dotnet) for registry PRs unless explicitly approved.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

risu729 · 2026-06-05T15:58:44Z

Closing this because current upstream aqua does not avoid this edge case either. I checked aquaproj/aqua at 1e4b1cf0: completeWindowsExtToAsset still appends the Windows extension unconditionally for format: raw after only skipping .exe and .jar, and a local RenderAsset reproduction rendered tool-1.0.0-windows-amd64.bat.bat.

I also scanned current aquaproj/aqua-registry at 830124e62b and found no packages matching the exact risky shape (format: raw with custom windows_ext and an already-extended Windows asset/URL).

This comment was generated by an AI coding assistant.

fix(aqua): avoid double raw windows extensions

ac34b8e

docs(aqua): clarify raw windows extension behavior

882fa7a

risu729 marked this pull request as ready for review June 5, 2026 15:58

risu729 closed this Jun 5, 2026

risu729 deleted the fix/aqua-raw-windows-ext branch June 5, 2026 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(aqua): avoid double raw windows extensions#10227

fix(aqua): avoid double raw windows extensions#10227
risu729 wants to merge 2 commits into
jdx:mainfrom
risu729:fix/aqua-raw-windows-ext

risu729 commented Jun 4, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 4, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

risu729 commented Jun 4, 2026

Uh oh!

coderabbitai Bot commented Jun 4, 2026

Uh oh!

greptile-apps Bot commented Jun 4, 2026 •

edited

Loading

Uh oh!

risu729 commented Jun 5, 2026

Uh oh!

coderabbitai Bot commented Jun 5, 2026 •

edited

Loading

Uh oh!

risu729 commented Jun 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

risu729 commented Jun 4, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Review validation

Upstream aqua comparison

Tests

Current aqua-registry packages

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

risu729 commented Jun 4, 2026

Uh oh!

coderabbitai Bot commented Jun 4, 2026

Review of fix(aqua): avoid double raw windows extensions

Logic

Correctness of os_file_ext_is_empty

Edge-case interaction with early guards

Test coverage

No concerns

Uh oh!

greptile-apps Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

risu729 commented Jun 5, 2026

Uh oh!

coderabbitai Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

risu729 commented Jun 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

risu729 commented Jun 4, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 4, 2026 •

edited

Loading

Review of `fix(aqua): avoid double raw windows extensions`

Correctness of `os_file_ext_is_empty`

greptile-apps Bot commented Jun 4, 2026 •

edited

Loading

coderabbitai Bot commented Jun 5, 2026 •

edited

Loading